-
Notifications
You must be signed in to change notification settings - Fork 121
[Mobile Payments] Add Tap on Mobile entitlement back to build #8273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This entitlement is now added to our development provisioning profiles, so it’s safe to add it back to trunk without breaking dev environments for people.
You can test the changes from this Pull Request by:
|
|
Hi! I took some time to review this PR and found some issues (let me know if you'd like them turned into individual GitHub issues or if you just want them here). I recognize that some of the things I may have found may not have been in scope yet, but would rather assume more is in scope than less so we don't miss things!
Steps:
Steps:
RPReplay_Final1669922756.MP4 |
|
Adding a few more observations from early testing, some are UI issues:
|
|
Thanks @lmischner! I appreciate your work here, it helps keep us all aligned and understanding of where the progress is at, so thanks for doing it and recognising that some of these things might just not be done yet.
Yep, I'm aware of this one. I was going to tackle it under #8089, but actually it's probably a different problem so I've raised #8288
Agreed. There are issues with the bluetooth reader connection flow too which cause this, though they're more subtle and not as easy/likely to exploit, because you're switching from one device to another. In particular, the same issue as you raise here: we continue to show "Collect Payment" long after the Stripe SDK has collected the payment details, right up until we send the authorised payment to WCPay servers. I've got some ideas on why that is and how we can fix it, but hadn't logged them anywhere. Added #8289
This looks like a mistake on my part. The intention is that the Manage Card Reader screen should not attempt to discover to a built in reader (because there's nothing to manage), but should show when one is already connected (so that it can be disconnected and a bluetooth reader connected instead.) I can see that it's doing a search for the connected card reader when the feature flag is enabled, which is wrong. I'll sort that on the latest PR in this stack today. Thanks!
This will be covered under #8089
Nice catch! It only seems to happen when the second store isn't set-up for Tap on iPhone, it lets me take a payment if I try and repro it using two stores which I've been through the set-up flow for already. My first instinct was that we'd need to disconnect the reader when the store changes... but I was surprised that's not already handled for bluetooth readers. It looks like it is handled, as we're not connected to the built-in reader when we go to the set-up flow for the second reader. I checked what happens when I disconnect the built-in reader in It looks like a tricky one, and it may even be something in the Stripe SDK. This may take some investigating time so I'll log it and maybe look at it next week. I don't want to get distracted by an edge case this early, but it's really good to know about it. Logged #8290 |
|
Thanks for the review @jostnes!
This screen is removed from the connection flow in the latest PR: we auto connect to the built-in reader in that flow, so no need to show this one any more.
These are covered by #8081
This is expected: the flag controls the reader that they're connecting to, but we don't connect to a reader if there's already one connected. Using the |
This is fixed in #8281 now. |
|
Thanks for the PR @joshheald ! It looks great. As discussed, I retrieved a couple of unhandled errors, that should be tackled in this issue. Apart from that, all good. |
Description
In #8212, I removed the
com.apple.developer.proximity-reader.payment.acceptanceentitlement from the app, as I had mistakenly merged it to trunk in #8172, before the entitlement was added to our development provisioning profiles. This meant that people with the latest provisioning profiles would have been unable to build the app on their devices.This PR adds the entitlement back to the app, for debug and release flavours. The entitlement is now added to our main App ID, and our development provisioning profiles, so it’s safe to add it back to trunk without breaking dev environments for people.
It's not added on the alpha entitlements, as our Enterprise App ID doesn't have the entitlement added by Apple. We're unsure whether they'll grant it on an Enterprise app, so that's being treated as a separate request.
No rush on this: I've put it in next week's milestone
Testing instructions
Using a build from Xcode, on a physical iPhone (the feature is not supported on iPad.)
You'll need to use an iPhone XS or newer, running iOS 15.4 or above.
Menu > Settings > Experimental featuresTap to Pay on iPhoneMenu > Payments > Collect paymentCardon the payment method screenTap to Pay on iPhoneand go through the Terms of Service Apple ID linking (if you've not done so before)N.B. much of the flow is still under development, so it's not at all polished!
RELEASE-NOTES.txtif necessary.